Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(pageserver): persist aux file policy in index part #7668

Merged
merged 21 commits into from
May 17, 2024
Merged

feat(pageserver): persist aux file policy in index part #7668

merged 21 commits into from
May 17, 2024

Conversation

skyzh
Copy link
Member

@skyzh skyzh commented May 8, 2024 •
edited
Loading

Problem

Part of #7462

Summary of changes

Tenant config is not persisted unless it's attached on the storage controller. In this pull request, we persist the aux file policy flag in the index_part.json.

Admins can set switch_aux_file_policy in the storage controller or using the page server API. Upon the first aux file gets written, the write path will compare the aux file policy target with the current policy. If it is switch-able, we will do the switch. Otherwise, the original policy will be used. The test cases show what the admins can do / cannot do.

The last_aux_file_policy is stored in IndexPart. Updates to the persisted policy are done via schedule_index_upload_for_aux_file_policy_update. On the write path, the writer will update the field.

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

@skyzh skyzh requested a review from a team as a code owner May 8, 2024 19:32
@skyzh skyzh requested review from koivunej and removed request for a team May 8, 2024 19:32
@skyzh skyzh force-pushed the skyzh/aux-file-flag-v2-again-2 branch from 876a814 to 41fd310 Compare May 8, 2024 19:33
@skyzh skyzh mentioned this pull request May 8, 2024
Closed
24 tasks
@skyzh skyzh force-pushed the skyzh/aux-file-flag-v2-again-2 branch from 41fd310 to 6401330 Compare May 8, 2024 19:36
@github-actions GitHub Actions
Copy link

github-actions bot commented May 8, 2024 •
edited
Loading

3096 tests run: 2969 passed, 0 failed, 127 skipped (full report)

Flaky tests (3)

Postgres 16

  • test_vm_bit_clear_on_heap_lock: debug
  • test_wal_restore: debug
  • test_wal_restore_initdb: debug

Code coverage* (full report)

  • functions: 31.6% (6388 of 20214 functions)
  • lines: 47.9% (48654 of 101533 lines)

* collected from Rust tests only

The comment gets automatically updated with the latest test results
24a864a at 2024-05-17T19:30:46.315Z :recycle:

@skyzh skyzh force-pushed the skyzh/aux-file-flag-v2-again branch from 03038da to 396b9cf Compare May 9, 2024 17:41
@skyzh skyzh force-pushed the skyzh/aux-file-flag-v2-again-2 branch 2 times, most recently from 2f73656 to 2ee9021 Compare May 9, 2024 18:49
@skyzh skyzh requested a review from jcsp May 9, 2024 18:50
koivunej
koivunej requested changes May 10, 2024
View reviewed changes
Copy link
Member

@koivunej koivunej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-emptively for https://github.com/neondatabase/neon/pull/7668/files#r1596792568.

@skyzh skyzh force-pushed the skyzh/aux-file-flag-v2-again-2 branch 2 times, most recently from 93b234b to abbdfda Compare May 13, 2024 19:17
@skyzh
Copy link
Member Author

skyzh commented May 13, 2024

Updated the pull request with storing the field in index part, and waiting for CI runs. The pull request body also gets updated correspondingly.

@skyzh skyzh changed the base branch from skyzh/aux-file-flag-v2-again to main May 13, 2024 19:26
@skyzh skyzh requested review from a team as code owners May 13, 2024 19:26
@skyzh skyzh requested review from MMeent and petuhovskiy May 13, 2024 19:26
@skyzh skyzh force-pushed the skyzh/aux-file-flag-v2-again-2 branch from abbdfda to 41453ac Compare May 13, 2024 19:26
@skyzh skyzh removed request for a team, MMeent and petuhovskiy May 13, 2024 19:27
@skyzh
fix tests
a2c91d6 
Signed-off-by: Alex Chi Z <[email protected]>
@skyzh skyzh requested a review from koivunej May 15, 2024 19:55
koivunej
koivunej reviewed May 16, 2024
View reviewed changes
pageserver/src/tenant.rs Outdated Show resolved Hide resolved
koivunej
koivunej reviewed May 16, 2024
View reviewed changes
pageserver/src/tenant.rs Outdated Show resolved Hide resolved
koivunej
koivunej reviewed May 16, 2024
View reviewed changes
pageserver/src/tenant.rs Outdated Show resolved Hide resolved
skyzh and others added 6 commits May 16, 2024 11:27
@skyzh
Copy link
Member Author

skyzh commented May 16, 2024

I also took the test cases from #7781 and adapted to my proposed semantics of these flags.

@skyzh
Copy link
Member Author

skyzh commented May 16, 2024

And to add, I think the main confusing in this PR is that we don't plan to support v1->v2 migration for customers. New customers go into cross validation mode first and go into v2 when we remove the read/write path of v1; existing customers will jump start from v2 and not being able to access their v1 replication state. v1->v2 migration is possible but it needs careful design, and with the persistent flag, it is still possible for us to do so, but I decided not to do that for now.

@skyzh
add default_tenant_config
61b28ba 
Signed-off-by: Alex Chi Z <[email protected]>
@skyzh
Copy link
Member Author

skyzh commented May 16, 2024

added AuxFilePolicy::default_tenant_config instead of implementing Default to make the semantics clear given that sometimes None is the real default value.

@skyzh skyzh requested a review from koivunej May 16, 2024 18:12
@skyzh
nit
76d4e77 
Signed-off-by: Alex Chi Z <[email protected]>
@skyzh
rm seqcst comment
24a864a 
Signed-off-by: Alex Chi Z <[email protected]>
@skyzh skyzh enabled auto-merge (squash) May 17, 2024 19:02
@skyzh skyzh merged commit aaf6081 into main May 17, 2024
54 of 55 checks passed
@skyzh skyzh deleted the skyzh/aux-file-flag-v2-again-2 branch May 17, 2024 19:22
a-masterov pushed a commit that referenced this pull request May 20, 2024
@skyzh @koivunej @a-masterov
feat(pageserver): persist aux file policy in index part (#7668)
21ef21b 
Part of #7462

## Summary of changes

Tenant config is not persisted unless it's attached on the storage
controller. In this pull request, we persist the aux file policy flag in
the `index_part.json`.

Admins can set `switch_aux_file_policy` in the storage controller or
using the page server API. Upon the first aux file gets written, the
write path will compare the aux file policy target with the current
policy. If it is switch-able, we will do the switch. Otherwise, the
original policy will be used. The test cases show what the admins can do
/ cannot do.

The `last_aux_file_policy` is stored in `IndexPart`. Updates to the
persisted policy are done via
`schedule_index_upload_for_aux_file_policy_update`. On the write path,
the writer will update the field.

---------

Signed-off-by: Alex Chi Z <[email protected]>
Co-authored-by: Joonas Koivunen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MMeent MMeent MMeent left review comments

@koivunej koivunej koivunej approved these changes

@jcsp jcsp jcsp left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@skyzh @jcsp @koivunej @MMeent